Skip to content

fix: preserve preconfigured logging#755

Open
schultzjack wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
schultzjack:codex/388-preserve-configured-logging
Open

fix: preserve preconfigured logging#755
schultzjack wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
schultzjack:codex/388-preserve-configured-logging

Conversation

@schultzjack

@schultzjack schultzjack commented Jun 17, 2026

Copy link
Copy Markdown

Summary

  • track successful configure_logging() calls in the logging module
  • skip interface default logging setup when logging was already configured
  • add regression coverage for DataDesigner() preserving a prior debug logging config

Testing

  • uv run --group dev pytest packages/data-designer-config/tests/test_logging.py packages/data-designer/tests/interface/test_data_designer.py -q
  • make check-config check-interface
  • make test-config test-interface

Closes #388

Signed-off-by: schultzjack <schultzjack@users.noreply.github.com>
@schultzjack schultzjack requested a review from a team as a code owner June 17, 2026 00:17
@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where DataDesigner.__init__ would silently overwrite a caller's pre-configured logging setup (e.g. debug level) with the interface's default INFO config. The fix introduces a module-level _logging_configured flag in logging.py that is set after a successful configure_logging() call, and guards the default-logging call in _initialize_interface_runtime() behind an is_logging_configured() check.

  • logging.py: adds _logging_configured = False, sets it to True at the end of configure_logging(), and exposes is_logging_configured().
  • data_designer.py: wraps the default configure_logging() call in _initialize_interface_runtime() with if not is_logging_configured() so any prior configuration is preserved.
  • Tests: updates test_initialize_interface_runtime_runs_once to reset _logging_configured via monkeypatch (keeping the mock assertion reliable), and adds two new tests — one unit test for the flag itself, and one end-to-end regression that verifies a DEBUG config survives DataDesigner() construction.

Confidence Score: 5/5

Safe to merge — the change is minimal, targeted, and fully covered by new and updated tests.

The fix is a one-flag, one-guard change with no impact on the happy path (logging not pre-configured) and correct behaviour on the new path (logging pre-configured). The existing test_initialize_interface_runtime_runs_once was correctly patched to reset the new flag so its mock assertion stays valid. The regression test exercises the real code path end-to-end and cleans up logger state in a finally block.

No files require special attention.

Important Files Changed

Filename Overview
packages/data-designer-config/src/data_designer/logging.py Adds module-level _logging_configured flag, sets it at the end of configure_logging(), and exposes is_logging_configured() predicate — straightforward and correct.
packages/data-designer/src/data_designer/interface/data_designer.py Guards the default configure_logging() call in _initialize_interface_runtime() behind is_logging_configured() — clean one-line change with no unintended side effects.
packages/data-designer-config/tests/test_logging.py Adds a focused test for the new _logging_configured flag using monkeypatch for proper isolation.
packages/data-designer/tests/interface/test_data_designer.py Adds regression test for preconfigured-logging preservation and fixes the existing test_initialize_interface_runtime_runs_once by resetting _logging_configured — necessary to keep the mock assertion reliable.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant User
    participant logging as data_designer.logging
    participant iface as _initialize_interface_runtime

    User->>logging: configure_logging(LoggingConfig.debug())
    logging->>logging: set handlers, levels
    logging->>logging: "_logging_configured = True"

    User->>iface: DataDesigner(...)
    iface->>iface: _interface_runtime_initialized? → False
    iface->>logging: is_logging_configured()
    logging-->>iface: True
    iface->>iface: skip configure_logging() ✓
    iface->>iface: resolve_seed_default_model_settings()
    iface->>iface: "_interface_runtime_initialized = True"
    iface-->>User: DataDesigner ready (DEBUG logging preserved)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant User
    participant logging as data_designer.logging
    participant iface as _initialize_interface_runtime

    User->>logging: configure_logging(LoggingConfig.debug())
    logging->>logging: set handlers, levels
    logging->>logging: "_logging_configured = True"

    User->>iface: DataDesigner(...)
    iface->>iface: _interface_runtime_initialized? → False
    iface->>logging: is_logging_configured()
    logging-->>iface: True
    iface->>iface: skip configure_logging() ✓
    iface->>iface: resolve_seed_default_model_settings()
    iface->>iface: "_interface_runtime_initialized = True"
    iface-->>User: DataDesigner ready (DEBUG logging preserved)
Loading

Reviews (1): Last reviewed commit: "fix: preserve preconfigured logging" | Re-trigger Greptile

@schultzjack

Copy link
Copy Markdown
Author

recheck

@github-actions

Copy link
Copy Markdown
Contributor

Stale PR reminder

This PR has had failing checks for 7 days without activity.

Failing checks: check

Please push an update or leave a comment if you're still working on this.
Otherwise, this PR will be automatically closed in 7 days.

To prevent auto-close, add the keep-open label.

@github-actions

Copy link
Copy Markdown
Contributor

Issue #388 has been triaged. The linked issue check is being re-evaluated.

@andreatgretel

Copy link
Copy Markdown
Contributor

Thanks for jumping on this, this fixes a pretty easy-to-miss logging footgun. I reviewed the runtime init path and the new logging state tracking. The core behavior looks good, but I'd like one small clarification before merge.

is_logging_configured() only tracks calls through data_designer.logging.configure_logging(). It won't detect users who configured logging through the standard Python APIs, like logging.basicConfig() or manually attached root handlers, so DataDesigner() would still call configure_logging() and replace those handlers.

Please add a short docstring/comment making that scope explicit, or broaden the check if you want this to cover general Python logging setup too. The current fix is fine for the linked issue, but the helper name reads a bit broader than what it actually guarantees.

Focused tests and a smoke check passed locally. Once that expectation is tightened up, this looks good to merge from my side.

@andreatgretel

Copy link
Copy Markdown
Contributor

Following up on my comment above: the current fix handles the narrow repro from #388 where the caller uses data_designer.logging.configure_logging() before constructing DataDesigner.

There is a related case discussed later in #388 that this does not cover yet: applications that configure logging through stdlib APIs before using Data Designer. For example, if a caller uses logging.basicConfig() or attaches custom root handlers, DataDesigner() can still call DD's default configure_logging(), clear those root handlers, and replace the application's logging setup.

I think the lowest-risk incremental direction is to make DD's automatic runtime logging setup opt-out explicitly, which also matches the direction Mike suggested on #388. Something like:

  • add an auto_configure_logging: bool = True keyword to DataDesigner.__init__
  • pass it into _initialize_interface_runtime(...)
  • only call DD's default configure_logging() when both auto_configure_logging is True and DD logging has not already been configured
  • add tests for:
    • default behavior still configures DD logging
    • DataDesigner(..., auto_configure_logging=False) preserves existing root handlers
    • prior data_designer.logging.configure_logging(...) is still preserved

That should keep standalone behavior unchanged while giving embedded/library users a reliable way to prevent DD from taking over process logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

configure_logging() called before DataDesigner() is silently overwritten

2 participants